-
Notifications
You must be signed in to change notification settings - Fork 258
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow for Redis environment variables #141
base: master
Are you sure you want to change the base?
Conversation
This allows for you to run your storage anywhere, as the host name is not often just redis. This change will make no impact on exisiting users as it allows for the env var to be set or the defaults as set before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alanakirby,
This will be useful flexibility to have - thanks for pointing this out! Please could you look at a comment I've left to show a way in which we can use the existing code for grabbing from environment variables to help us do this in the same way as other settings variables?
REDIS_HOST=os.environ('REDIS_HOST', 'redis') | ||
REDIS_PORT=os.environ('REDIS_PORT', '6379') | ||
REDIS_DB=os.environ('REDIS_DB', '0') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's make this use the existing code to configure settings by setting environment variables. To do that, you can add these REDIS_HOST
, REDIS_PORT
and REDIS_DB
to the existing loop in
for envvar in ['SMTP_PORT', 'SMTP_USERNAME', 'SMTP_PASSWORD',
...
And then to set the defaults after the loop, set the defaults if not already set.
if not REDIS_HOST:
REDIS_HOST='redis'
if not REDIS_PORT:
...
The settings module is clumsy at the moment at setting defaults, but sticking to the convention of having environment variables formatted like CANARY_REDIS_HOST
and CANARY_REDIS_PORT
using existing code, will help us when we refactor this module to make these types of changes easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @alanakirby,
Just checking you saw this response. We would like to get this merged in 🤘
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Jay! Its been so long since Ive worked on this project actually. Would your suggestion above allow for storage other than redis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alanakirby, I'm unsure this commit should attempt to do that. The reason I say this is because we use redis
specific libraries through the codebase and so the redis
python is unlikely to work with other storages. Which storages did you have in mind?
This allows for you to run your storage anywhere, as the host name is not often just redis. This change will make no impact on exisiting users as it allows for the env var to be set or the defaults as set before.